Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for chunking upload #212

Merged
merged 2 commits into from
Jun 6, 2018
Merged

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented May 30, 2018

Test for #198
Requires owncloud/core#31596 to be merged

@patrickjahns
Copy link
Contributor

@IljaN - please:

  • add respective labels
  • assign yourself as being responsible when creating the PR
  • add to milestone

On the content - did you check that the behat tests will transform the upload to chunking?

Checking core feature files for webdav - we explicitly chunk: https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdav/webdav-related-new-endpoint.feature#L464-L473

something to consider - instead of uploading a large file, create the file during the test run - so people do not have to check out unecessary large github repositories

@PVince81
Copy link
Contributor

PVince81 commented May 30, 2018

also, as discussed in the core PR:

  • add a test that does a DELETE on the uploads folder for cancellation, to confirm there is no permission issue

@IljaN IljaN added this to the development milestone Jun 1, 2018
@IljaN IljaN changed the title test for chunking upload Test for chunking upload Jun 1, 2018
@IljaN
Copy link
Member Author

IljaN commented Jun 1, 2018

There seems to be no pre-defined step to delete files outside of /$user/files, should i create one? @individual-it, @phil-davis

Maybe an abstraction step for the chunking process should be provided which does the whole process without needing to do it manually like this:

  @mailhog
  Scenario: A guest user can upload chunked files
    Given as user "admin"
    And user "user0" has been created
    And user "admin" has created guest user "guest" with email "[email protected]"
    And the HTTP status code should be "201"
    And user "user0" has created a folder "/tmp"
    And user "user0" has shared folder "/tmp" with user "[email protected]"
    And guest user "guest" has registered
    When user "[email protected]" creates a new chunking upload with id "chunking-42" using the API
    And user "[email protected]" uploads new chunk file "1" with "AAAAA" to id "chunking-42" using the API
    And user "[email protected]" uploads new chunk file "2" with "BBBBB" to id "chunking-42" using the API
    And user "[email protected]" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the API
    And user "[email protected]" moves new chunk file with id "chunking-42" to "/tmp/myChunkedFile.txt" using the API
    Then as "[email protected]" the file "/tmp/myChunkedFile.txt" should exist
    When user "[email protected]" deletes folder "/uploads/chunking-42" using the API
    Then as "[email protected]" the file "/uploads/chunking-42" should not exist

@IljaN
Copy link
Member Author

IljaN commented Jun 1, 2018

@PVince81 test added

@IljaN IljaN force-pushed the chunking_upload_acceptance_test branch from 4bd9d13 to 5361d39 Compare June 1, 2018 13:13
@IljaN IljaN added the bug label Jun 1, 2018
@phil-davis
Copy link
Contributor

phil-davis commented Jun 1, 2018

@IljaN yes, compressing that list of chunking steps would be nice for "routine" chunking tests - it can assume some stuff and do it all. If you want detailed control (upload weird numbers of chunks of different sizes out-of-order,...) then you can write it out as individual steps.
I will make a PR to provide that.
owncloud/core#31616

But for now the long-winded steps work.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I suggested a few little extra checks that we are starting to add these days - making sure that the HTTP status returned by requested did not lie.

And user "[email protected]" uploads new chunk file "2" with "BBBBB" to id "chunking-42" using the API
And user "[email protected]" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the API
And user "[email protected]" moves new chunk file with id "chunking-42" to "/tmp/myChunkedFile.txt" using the API
Then as "[email protected]" the file "/tmp/myChunkedFile.txt" should exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also add:

And as "user0" the file "/tmp/myChunkedFile.txt" should exist

just to be sure it ended up where it should.

And user "[email protected]" uploads new chunk file "2" with "BBBBB" to id "chunking-42" using the API
And user "[email protected]" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the API
And user "[email protected]" cancels chunking-upload with id "chunking-42" using the API
Then the HTTP status code should be "204"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are feeling like really making sure, you can say:

And as "user0" the file "/tmp/myChunkedFile.txt" should not exist

to be sure the file did not arrive. (Make sure the HTTP status did not lie)
(it seems odd to me that the cancel of chunking is status 204 - but I guess that indicates the cancel was successful)

@patrickjahns
Copy link
Contributor

patrickjahns commented Jun 1, 2018

@IljaN
restarted - even though it is now green for stable10 - these errors strike me - cc @PVince81

[Fri Jun  1 23:26:28 2018] Storage wrapper 'oc_readonly' was not registered via the 'OC_Filesystem - preSetup' hook which could cause potential problems.
[Fri Jun  1 23:26:28 2018] Could not get node for path: "uploads/[email protected]/chunking-42/.file" : File with name //chunking-42 could not be located

@PVince81 PVince81 assigned PVince81 and unassigned IljaN Jun 4, 2018
@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

first warning is expected, needs fixing in core at some point.

second one is strange... I'll have a look

@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

hmmm, also something missing on master ? (from Drone results)


--- GuestsContext has missing steps. Define them with these snippets:

    /**
     * @When user :arg1 cancels chunking-upload with id :arg2 using the API
     */
    public function userCancelsChunkingUploadWithIdUsingTheApi($arg1, $arg2)
    {
        throw new PendingException();
}

@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

@patrickjahns
Copy link
Contributor

@PVince81 - missing master as the required tests step implementation is in core

@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

@patrickjahns I'm not seeing the second error from your comment #212 (comment) when running tests locally. Was it from a specific drone run ?

@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

@patrickjahns ok I see the error on Drone: https://drone.owncloud.com/owncloud/guests/18/43 line 190.

Since Drone runs with the daily tarball I should try with that one locally as well.

@PVince81
Copy link
Contributor

PVince81 commented Jun 4, 2018

@patrickjahns I managed to reproduce this, I wasn't looking properly. Using a search for ".file" helped me find it in the local log.

It's a core issue: owncloud/core#31631

Some of our Sabre plugins is trying to clean up custom properties and tries to get the node after it was already moved, so it doesn't find it. Nothing to worry about in the context of uploads, but I mentioned in the ticket that we should check whether a real issue might exist with regular files and custom properties.

@PVince81
Copy link
Contributor

PVince81 commented Jun 5, 2018

added more verification steps as advised by @phil-davis

master got the acceptance test step so I expect everything to be green on all branches now

@phil-davis
Copy link
Contributor

Looks good, assuming CI passes.

@PVince81
Copy link
Contributor

PVince81 commented Jun 5, 2018

daily-master-qa... need to wait for tomorrow as it will contain the fix. stay tuned

@PVince81
Copy link
Contributor

PVince81 commented Jun 6, 2018

restarted failed builds, expecting success with today's daily-master-qa

@PVince81 PVince81 merged commit d656866 into master Jun 6, 2018
@PVince81 PVince81 deleted the chunking_upload_acceptance_test branch June 6, 2018 16:54
@PVince81 PVince81 modified the milestones: development, QA Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants